Skip to content

Conversation

@sago35
Copy link
Member

@sago35 sago35 commented May 20, 2020

I added Seeed WioTerminal support to TinyGo.
https://wiki.seeedstudio.com/Wio-Terminal-Getting-Started/

ATSAMD5x DataSheet: https://files.seeedstudio.com/wiki/Wio-Terminal/res/ATSAMD51.pdf
PinOut: https://files.seeedstudio.com/wiki/Wio-Terminal/img/WioT-Pinout.jpg
Schematics: https://files.seeedstudio.com/wiki/Wio-Terminal/res/Wio-Terminal-Schematics.pdf

  • Feature
    • ATSAMD51P19 (120MHz / 128 Pins / 512KB / 192KB)
    • 4MB External Flash
    • Realtek RTL8720DN (Dual Band 2.4GHz / 5GHz WiFi, BLE 5.0)
    • USB OTG
    • LCD, WIFI, BT, IMU, Microphone, Speaker, microSD Card, Light Sensor, 5-Way Switch, Infrared Emitter (IR 940nm), Crypto-authentication

Current Status:

  • status

    • 4MB External Flash
      • tinygo.org/x/drivers/examples/flash/console/qspi
    • WiFi / BT
      • not for this PR
    • USB OTG
      • not for this PR
    • src/examples/adc
    • src/examples/blinky1
    • src/examples/button
    • src/examples/echo
    • src/examples/pininterrupt
    • src/examples/pwm
    • src/examples/serial
    • src/examples/systick
    • LCD
      • tinygo.org/x/drivers/examples/ili9341/basic
      • tinygo.org/x/drivers/examples/ili9341/pyportal_boing
      • tinygo.org/x/drivers/examples/ili9341/scroll
    • IMU
      • tinygo.org/x/drivers/examples/lis3dh
    • Microphone
      • MIC input (PC30 port ADC)
    • Speaker
      • tinygo.org/x/drivers/examples/buzzer
    • microSD Card
    • Light Sensor
      • PD01 port ADC
    • 5-Way Switch and 3 x buttons
      • PD20, PD12, PD09, PD08, PD10
      • PC26, PC27, PC28
    • Infrared Emmitter (IR 940nm)
      • PB31
    • ATECC608 : I2C CrypitoAuthentication Device
      • not for this PR It wasn't implemented on the board.
  • TODO

Comment on lines 12 to 27
D0 = PB17 // UART0 RX/PWM available
D1 = PB16 // UART0 TX/PWM available
D4 = PA14 // PWM available
D5 = PA16 // PWM available
D6 = PA18 // PWM available
D8 = PB03 // built-in neopixel
D9 = PA19 // PWM available
D10 = PA20 // can be used for PWM or UART1 TX
D11 = PA21 // can be used for PWM or UART1 RX
D12 = PA22 // PWM available
D13 = PA23 // PWM available
D21 = PA13 // PWM available
D22 = PA12 // PWM available
D23 = PB22 // PWM available
D24 = PB23 // PWM available
D25 = PA17 // PWM available
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not easily find these in the schematics. Are these pins available on the WioTerminal?

Copy link
Member Author

@sago35 sago35 May 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't made it to the correct value yet.

The schematic and pinouts can be found below.
https://files.seeedstudio.com/wiki/Wio-Terminal/res/Wio-Terminal-Schematics.pdf
https://files.seeedstudio.com/wiki/Wio-Terminal/img/WioT-Pinout.jpg

Is the definition of Dxx correct to set D0 to D8 in the Pinout Diagram?
Is it right to fit the Arduino?

Eventually, I'm going to adjust to the following.

https://github.com/Seeed-Studio/ArduinoCore-samd/blob/master/variants/wio_terminal/variant.h
https://github.com/Seeed-Studio/ArduinoCore-samd/blob/master/variants/wio_terminal/variant.cpp

For example.

#define D0 (0ul)
const PinDescription g_APinDescription[0] =
        // 0..8 -  Raspberry pi Digital & AD pins
        {PORTB, 8, PIO_ANALOG, (PIN_ATTR_ANALOG | PIN_ATTR_PWM_E), ADC_Channel2, TC4_CH0, TC4_CH0, EXTERNAL_INT_8}, // 0/RPI_D0/RPI_A0
        }

I'm going to set it as follows.

D0  = PB08

SPI0_MISO_PIN = PB00 // MISO: SERCOM5/PAD[2]
)

// SPI on the Feather M4.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you copied this from the Feather M4?

Are you sure that all the definitions in this file are correct for the WioTerminal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@aykevl aykevl May 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We follow the board markings in TinyGo. If there are no markings on the bard, using the same pin names as Arduino or MicroPython use is fine.

}

// Transfer2 writes/reads a single uint16 using the SPI interface.
func (spi SPI) Transfer2(w uint16) (uint16, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you have extended the SPI peripheral with 16-bit and 32-bit methods.

  1. Does this provide an improvement in speed?
  2. Is this really necessary for WioTerminal support?

I'm asking this, because eventually I would like to switch to DMA and interrupt based SPI transfers. That would make it unnecessary to add Transfer2 and Transfer4 methods, as all the traffic is handled by DMA (which is the fastest possible transfer, except maybe for very short transfers).

Copy link
Member Author

@sago35 sago35 May 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this provide an improvement in speed?
Is this really necessary for WioTerminal support?

Yes.

In the demo of pyportal_boing, ILI9341 is too slow at about 10 fps when using Transfer().
Using Transfer2() makes the ILI9341 32 fps and Transfer4() makes it 36 fps.

The processing time excluding DrawRGBBitmap() was 12.5ms, so the time it takes for one DrawRGBBitmap() is as follows

  • Transfer : 100 ms
  • Transfer2 : 31.25 ms
  • Transfer3 : 27.8 ms

Also, I think that DMA should be enabled at some point.
But without a DMA, it's too slow to use Trasnfer4.
So I'd like to enable Transfer4 once, if possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may be able to use a trick like here to improve the speed:
#562
What it does, is that it makes use of double buffering support in the hardware. This makes it possible to continuously stream out data. Maybe the SAM D5x also supports double buffering in the DATA register.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By using INTEFLAG.DRE, I was able to improve performance without using 32bit-extensions.
ILI9341's pyportal_boing is now 43 fps instead of 36 fps.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ILI9341's pyportal_boing is now 43 fps instead of 36 fps.

I think you possibly meant "36 fps instead of 43 fps" 😸

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double-buffer-like processing of DRE checks seems to be faster than the 4-byte writes ( the 32B-extensions).

board cpu clock driver fps
pyportal samd51j20 120Mhz parallel 51 fps
wioterminal samd51p19 120Mhz spi (first commit) 10 fps
wioterminal samd51p19 120Mhz spi with 32B extensions 36 fps
wioterminal samd51p19 120Mhz spi with DRE check and without 32B extensions 43 fps

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a nice improvement! And it should benefit anyone using SPI on the atsamd51.
Can you make a separate PR with those changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, now I see you already made the change in tinygo-org/drivers#153.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the case of just sending (spi.Tx(w, nil)) can be done faster in the same way.
I put that code in ILI9341, but I think it can be done by moving it from spi.go to machine_atsamd51.go (maybe atsamd21 can do it the same way).

@sago35
Copy link
Member Author

sago35 commented May 21, 2020

What should RESET_MAGIC_VALUE be?

@deadprogram
Copy link
Member

The address RESET_MAGIC_VALUE is used by the bootloader to trigger the mode to receive new code. Do you happen to know where the source code for the uf2 bootloader used by the WioTerminal?

https://github.com/adafruit/uf2-samdx1/blob/master/inc/uf2.h#L249 is where the original value that we are using comes from.

@deadprogram
Copy link
Member

@sago35 can you please rebase against the dev branch? That should allow all of the tests to pass now.

@sago35
Copy link
Member Author

sago35 commented May 23, 2020

The address RESET_MAGIC_VALUE is used by the bootloader to trigger the mode to receive new code. Do you happen to know where the source code for the uf2 bootloader used by the WioTerminal?

I think it's probably this.

https://github.com/Seeed-Studio/ArduinoCore-samd/blob/master/cores/arduino/Reset.cpp#L59

This meant that no changes were needed.
It can be rewritten in tinygo flash.
If I change the value, you won't be able to do the tinygo flash.

@deadprogram
Copy link
Member

Since it is the same value for the WioTerminal, no changes needed, you can just use the same value.

@sago35
Copy link
Member Author

sago35 commented May 24, 2020

@aykevl
When I try to configure UART1, the settings in machine_atsamd51.go and wioterminal are different.
I think I'll move this around to board_*.go, is that okay?

Maybe I'll change the file below.

@sago35
Copy link
Member Author

sago35 commented May 27, 2020

@deadprogram
I think that the remaining TODO is how to do UART1.
If no changes are needed, the PR is complete.

And apart from this PR, I'm working on the following

I'm hoping to make the following as well, but I think it will take some time.

  • RTL8720 wifi driver

@sago35 sago35 requested review from aykevl and deadprogram May 27, 2020 22:49
@sago35
Copy link
Member Author

sago35 commented May 27, 2020

I tried to write the following, but machine.UART1.handleInterrupt is a private function, so I got an error.
After all, it may be good to move the setting of UART1 to board_xxx.go.

error message: uart\main.go:37:75: machine.UART1.handleInterrupt undefined (type machine.UART has no field or method handleInterrupt)

func main() {
	uart1.Configure(machine.UARTConfig{TX: tx1, RX: rx1})
	machine.UART1 = machine.UART{
		Buffer: machine.NewRingBuffer(),
		Bus:    sam.SERCOM2_USART_INT,
		SERCOM: 2,
	}
	machine.UART1.Interrupt = interrupt.New(sam.IRQ_SERCOM2_2, machine.UART1.handleInterrupt)

@deadprogram
Copy link
Member

Hi @sago35 the interrupt handler for the UART would normally be in the machine package for that board, not in a user program. Hence why it is not exposed publicly.

Is there a reason to not add the UART1 implementation for the WioTerminal this way?

@sago35
Copy link
Member Author

sago35 commented May 28, 2020

Hi @deadprogram
(I'm not good at English, so I'm relying on translation tools)

#1124 (comment)
the interrupt handler for the UART would normally be in the machine package for that board, not in a user program. Hence why it is not exposed publicly.
Is there a reason to not add the UART1 implementation for the WioTerminal this way?

This implementation tried to see if it could override the UART1 settings without changing machine_atsamd51.go.
However, as a result, I still think it is necessary to change board_xxx.go because the setting cannot be overwritten.

I also think that UART is good with machine package at this point.
The problem that I am thinking about now is that UART1 of machine_atsamd51.go is set for itsybitsy-m4 only.

My suggestion is to move the setting of UART1 (and UART2 if necessary) to board_itsybitsy-m4.go or board_feather-m4.go etc. instead of machine_atsamd51.go.
Do you think this proposal is a good one?

For example, the setting of UART1 needs to be different for each board as follows.

itsybitsy-m4

	UART1 = UART{
		Buffer: NewRingBuffer(),
		Bus:    sam.SERCOM3_USART_INT,
		SERCOM: 3,
	}

	UART1.Interrupt = interrupt.New(sam.IRQ_SERCOM3_2, UART1.handleInterrupt)

wioterminal

	UART1 = UART{
		Buffer: NewRingBuffer(),
		Bus:    sam.SERCOM2_USART_INT,
		SERCOM: 2,
	}

	UART1.Interrupt = interrupt.New(sam.IRQ_SERCOM2_2, UART1.handleInterrupt)

@deadprogram
Copy link
Member

I think you have the correct idea. Please look at a similar SAMD21 implementation:

https://github.com/tinygo-org/tinygo/blob/master/src/machine/board_feather-m0.go#L57-L68

@sago35
Copy link
Member Author

sago35 commented May 28, 2020

OK, make SAMD51's UART in the same way.

@sago35
Copy link
Member Author

sago35 commented May 28, 2020

UART settings have been corrected.
I kept as much of the original settings as I could, but fixed the obviously different parts.

I have confirmed that feather-m4 and wioterminal work correctly.
The PyPortal is connected to ESP32 and is not sure if it can be configured correctly.
I don't have istybitsy-m4 and metro-m4-airlift and pybadge, so I can't test them.

@sago35
Copy link
Member Author

sago35 commented May 28, 2020

I think the only remaining task is to set up the interrupts pins.
If #1139 is merged immediately, this PR will address the interrupts pins.
If it takes a while to check #1139, set the SAMD51P19 interrupts pins in #1139.

@sago35
Copy link
Member Author

sago35 commented May 29, 2020

The following are now supported.

  • interrupts pins
  • arm: make FPU configuration consistent

@sago35
Copy link
Member Author

sago35 commented May 29, 2020

The only thing left to do is to

My source code changes are now complete.
I will force-push after #1141 is merged, but I don't think the source code will change.
Please review.
@deadprogram @aykevl

@sago35 sago35 changed the title [WIP] Seeed WioTerminal support Seeed WioTerminal support May 31, 2020
@sago35
Copy link
Member Author

sago35 commented May 31, 2020

This PR is now complete.
@deadprogram

@deadprogram
Copy link
Member

OK @sago35 thanks again for working on this. I have a few ideas on how to split this PR into more easily reviewable/mergable pieces:

  • Make the "Standardize SAMD51 UART Settings" dcbfb3d into a separate PR
  • Make "Extend SAMD51 pinPadMapping" 7b767d7 a separate PR
  • Make "Add SAMD51 ADC settings" d47bc08 into a separate PR

Once those are merged, probably the rest of the commits in this current PR can probably then just be squashed into a single PR that is "Add WioTerminal support"

What do you think?

@sago35
Copy link
Member Author

sago35 commented Jun 3, 2020

I will try to split the PR.

@sago35
Copy link
Member Author

sago35 commented Jun 3, 2020

Hi @deadprogram
I'm going to break it down into three PRs below.
git rebase worked well.
2 will be created after #1152 has been merged.

  1. add support for SAMD51 with 128 pins Add support for SAMD51 with 128 pins #1152
  2. Standardize SAMD51 UART Settings
  3. see WioTerminal support

@deadprogram
Copy link
Member

Now that #1155 has been merged, this branch needs rebasing to resolve merge conflicts. Thanks!

@sago35
Copy link
Member Author

sago35 commented Jun 5, 2020

Hi @deadprogram
Rebase is done.
The CI test was OK.
Please review.

@deadprogram
Copy link
Member

Thanks for all of the effort and changes to add this board @sago35 great work!

Now squash/merging. Thank you!

@deadprogram deadprogram merged commit c5a8967 into tinygo-org:dev Jun 6, 2020
@sago35 sago35 mentioned this pull request Jun 17, 2020
6 tasks
@niaow niaow added this to the v0.14 milestone Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants